Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

V2V - Collect virt-v2v PID from conversion host in kill_virtv2v #18372

Merged
merged 2 commits into from
Jan 18, 2019
Merged

V2V - Collect virt-v2v PID from conversion host in kill_virtv2v #18372

merged 2 commits into from
Jan 18, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 18, 2019

In the current implementation, we still use the old format of task.options[:virtv2v_wrapper] to identify the PID of virt-v2v. The value is always nil as we don't store it there anymore, which in turns makes the cancellation ineffective. This PR fixes it by calling get_conversion_state, that gets the state from the conversion host, and thus the PID.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1666799

@ghost ghost force-pushed the v2v_fix_virtv2v_kill branch from f58f079 to 8e5e70f Compare January 18, 2019 15:48
@ghost
Copy link
Author

ghost commented Jan 18, 2019

@miq-bot add-label transformation, bug, hammer/yes, blocker

@jameswnl
Copy link
Contributor

@fdupont-redhat why don't we store that anymore, since get_conversion_state is retrieving it already?

@ghost
Copy link
Author

ghost commented Jan 18, 2019

Because we get the whole state in VMCheckTransformed method. So, that's just an extra retrieval when we need to cleanup the conversion stuff.

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2019

Checked commits fabiendupont/manageiq@8e5e70f~...38a7994 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@agrare agrare self-assigned this Jan 18, 2019
@agrare
Copy link
Member

agrare commented Jan 18, 2019

@fdupont-redhat does the PID change over the course of a service_template_transformation_plan_task ? That seems like something we should be caching instead of doing another ssh call to get the state file.

@agrare
Copy link
Member

agrare commented Jan 18, 2019

Since this is a blocker I'll merge but @fdupont-redhat lets look at this refactoring in a follow-up, I'd rather not have to do an extra ssh call before the cancel

@agrare agrare merged commit 526045d into ManageIQ:master Jan 18, 2019
@agrare agrare added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 18, 2019
@ghost
Copy link
Author

ghost commented Jan 18, 2019

@agree I do agree that it could be cached, as the PID doesn't change once virt-v2v is started. It's a broader discussion about what should be stored in options has. Anyway, thanks for the merge.

simaishi pushed a commit that referenced this pull request Feb 5, 2019
@agrare @simaishi
Merge pull request #18372 from fdupont-redhat/v2v_fix_virtv2v_kill
0e878b7 
V2V - Collect virt-v2v PID from conversion host in kill_virtv2v

(cherry picked from commit 526045d)

https://bugzilla.redhat.com/show_bug.cgi?id=1672699
@simaishi
Copy link
Contributor

simaishi commented Feb 5, 2019

Hammer backport details:

$ git log -1
commit 0e878b781b1cfdf4b3cd5c35fbfb29578ad30679
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Jan 18 14:37:51 2019 -0500

    Merge pull request #18372 from fdupont-redhat/v2v_fix_virtv2v_kill
    
    V2V - Collect virt-v2v PID from conversion host in kill_virtv2v
    
    (cherry picked from commit 526045d22faae29ad8d74e4625c9f09206d5f97e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1672699

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@agrare agrare

Labels
blocker bug hammer/backported v2v
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jameswnl @miq-bot @agrare @simaishi @fabiendupont